fix(rootfs/qemu-static): use direct armhf exec probe instead of broken arch-test gate#9820
fix(rootfs/qemu-static): use direct armhf exec probe instead of broken arch-test gate#9820iav wants to merge 1 commit into
Conversation
iav
requested review from
ColorfulRhino and
schwar3kat
and removed request for
a team
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughProbe for native armhf execution; if absent, prefer a packaged ChangesQEMU ARMhf native capability probe and setup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
size/medium
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/functions/rootfs/qemu-static.sh (1)
232-233: 💤 Low valueHardcode
qemu-arminstead of inheritingwanted_archfrom caller scope.This function's name pins the target arch (
armhf_target), and the only call site (line 187) enters whenwanted_arch == "arm". Relying on the caller's loop variable here couples the helper to a particular invocation context without any guard — a future caller or refactor could silently produceqemu-with no arch suffix. Using the literal string also matches the heredoc above (line 222) which is already arm-specific.♻️ Suggested change
- run_host_command_logged update-binfmts --import "qemu-${wanted_arch}" - run_host_command_logged update-binfmts --enable "qemu-${wanted_arch}" + run_host_command_logged update-binfmts --import "qemu-arm" + run_host_command_logged update-binfmts --enable "qemu-arm"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/functions/rootfs/qemu-static.sh` around lines 232 - 233, The helper function armhf_target currently calls run_host_command_logged with "qemu-${wanted_arch}", inheriting wanted_arch from caller scope; change these two calls to use the literal "qemu-arm" instead of interpolating wanted_arch so the helper is self-contained and matches the arm-specific heredoc and intent. Update both run_host_command_logged update-binfmts --import and --enable invocations to use "qemu-arm" and ensure no other references inside armhf_target rely on wanted_arch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/functions/rootfs/qemu-static.sh`:
- Around line 232-233: The helper function armhf_target currently calls
run_host_command_logged with "qemu-${wanted_arch}", inheriting wanted_arch from
caller scope; change these two calls to use the literal "qemu-arm" instead of
interpolating wanted_arch so the helper is self-contained and matches the
arm-specific heredoc and intent. Update both run_host_command_logged
update-binfmts --import and --enable invocations to use "qemu-arm" and ensure no
other references inside armhf_target rely on wanted_arch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0f4f347d-a3de-47de-91fe-fa6e149fa4d8
📒 Files selected for processing (1)
lib/functions/rootfs/qemu-static.sh
iav
force-pushed
the
fix/native-armhf-detection
branch
2 times, most recently
from
934a2f0 to
d5b76d4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/functions/rootfs/qemu-static.sh (1)
218-226: 💤 Low valueProbe path is gated solely on the cross-toolchain loader, missing fallback for foreign architecture setups.
The probe at line 222 checks
/usr/arm-linux-gnueabihf/lib/ld-linux-armhf.so.3(installed bygcc-arm-linux-gnueabihf). If this cross-toolchain package is absent—possible in pre-prepared hosts or manual foreign architecture setups—the probe fails silently and falls through to write a hand-rolledqemu-armdescriptor and force emulation, defeating this PR's optimization.In normal builds,
gcc-arm-linux-gnueabihfis installed during host preparation when the target includes armhf, so this is not a typical correctness issue. However, hosts with armhf added as a foreign dpkg architecture (without the cross-toolchain) could use the alternative loader path/lib/arm-linux-gnueabihf/ld-linux-armhf.so.3to detect native capability without additional packages. Consider probing both paths to support this setup, or at minimum document thegcc-arm-linux-gnueabihfdependency at the call site so users debugging slow builds can identify whether they need to install it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/functions/rootfs/qemu-static.sh` around lines 218 - 226, The armhf kernel probe relies only on /usr/arm-linux-gnueabihf/lib/ld-linux-armhf.so.3 via the armhf_probe check and will miss hosts that have armhf as a foreign dpkg architecture where the loader lives at /lib/arm-linux-gnueabihf/ld-linux-armhf.so.3; update the probe logic used around the armhf_probe variable to check both paths ("/usr/arm-linux-gnueabihf/lib/ld-linux-armhf.so.3" and "/lib/arm-linux-gnueabihf/ld-linux-armhf.so.3") and invoke the same executable --help test for whichever exists, falling back to the existing qemu-arm descriptor logic only if neither path yields an executable; alternatively add a clear comment/log entry (via display_alert) documenting the gcc-arm-linux-gnueabihf dependency if you prefer not to add the second-path probe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/functions/rootfs/qemu-static.sh`:
- Around line 218-226: The armhf kernel probe relies only on
/usr/arm-linux-gnueabihf/lib/ld-linux-armhf.so.3 via the armhf_probe check and
will miss hosts that have armhf as a foreign dpkg architecture where the loader
lives at /lib/arm-linux-gnueabihf/ld-linux-armhf.so.3; update the probe logic
used around the armhf_probe variable to check both paths
("/usr/arm-linux-gnueabihf/lib/ld-linux-armhf.so.3" and
"/lib/arm-linux-gnueabihf/ld-linux-armhf.so.3") and invoke the same executable
--help test for whichever exists, falling back to the existing qemu-arm
descriptor logic only if neither path yields an executable; alternatively add a
clear comment/log entry (via display_alert) documenting the
gcc-arm-linux-gnueabihf dependency if you prefer not to add the second-path
probe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 688ab1df-44ba-4174-9a39-f917a6a0ebf8
📒 Files selected for processing (1)
lib/functions/rootfs/qemu-static.sh
iav
force-pushed
the
fix/native-armhf-detection
branch
from
d5b76d4 to
4be6288
Compare
|
Re: nitpick "Probe path is gated solely on the cross-toolchain loader, missing fallback for foreign architecture setups." Verified: The "absent" case the nitpick raises applies only when the operator sets |
|
Should we merge this after 05 release? |
igorpecovnik
added
08
|
I think it could be done earlier. |
|
Depends how well is this tested? I want as less surprises as possible when hitting the "build all" button. |
|
Indeed — my testing surface doesn't cover CI or the big builder fleet. Agree on holding the merge until builder hiccups can't threaten a release cut. |
Yep. After release we can afford to have broken CI for days while before its really not needed :) |
iav
force-pushed
the
fix/native-armhf-detection
branch
from
4be6288 to
53cb297
Compare
iav
force-pushed
the
fix/native-armhf-detection
branch
4 times, most recently
from
765d575 to
1aed440
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/functions/rootfs/qemu-static.sh (1)
261-274: 💤 Low valueConsider adding verification after hand-write path.
Unlike the packaged descriptor path (lines 239-244), there's no verification that
qemu-armactually became enabled in the kernel after import/enable. Whileset -ecatches command failures,update-binfmts --enablecould succeed without the kernel entry actually becoming enabled (e.g., if binfmt_misc is in an unexpected state).♻️ Optional: Add verification for consistency
run_host_command_logged update-binfmts --import qemu-arm run_host_command_logged update-binfmts --enable qemu-arm + if [[ ! -e /proc/sys/fs/binfmt_misc/qemu-arm ]] || + [[ "$(head -n1 /proc/sys/fs/binfmt_misc/qemu-arm 2> /dev/null)" != "enabled" ]]; then + exit_with_error "Hand-written qemu-arm descriptor could not be enabled. Check binfmt_misc state." + fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/functions/rootfs/qemu-static.sh` around lines 261 - 274, The hand-written qemu-arm descriptor path writes /usr/share/binfmts/qemu-arm and runs update-binfmts --import/--enable but lacks post-enable verification; after run_host_command_logged update-binfmts --enable qemu-arm add a check using update-binfmts --display qemu-arm or by reading /proc/sys/fs/binfmt_misc/qemu-arm (or /proc/sys/fs/binfmt_misc/status) to confirm the entry is present and marked enabled, and if the check fails retry or log an error and exit non-zero so the failure is not silently ignored (reference the created file /usr/share/binfmts/qemu-arm and the commands update-binfmts --import qemu-arm and update-binfmts --enable qemu-arm).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/functions/rootfs/qemu-static.sh`:
- Around line 276-279: The debug block is passing shell operators as literal
args to arch-test (run_host_command_logged arch-test "||" true); update the
if-body so the shell operator applies to the command exit instead of being
passed to arch-test—call run_host_command_logged arch-test and place || true
after that invocation (i.e., run_host_command_logged arch-test || true), keeping
the existing SHOW_DEBUG check and display_alert call unchanged.
---
Nitpick comments:
In `@lib/functions/rootfs/qemu-static.sh`:
- Around line 261-274: The hand-written qemu-arm descriptor path writes
/usr/share/binfmts/qemu-arm and runs update-binfmts --import/--enable but lacks
post-enable verification; after run_host_command_logged update-binfmts --enable
qemu-arm add a check using update-binfmts --display qemu-arm or by reading
/proc/sys/fs/binfmt_misc/qemu-arm (or /proc/sys/fs/binfmt_misc/status) to
confirm the entry is present and marked enabled, and if the check fails retry or
log an error and exit non-zero so the failure is not silently ignored (reference
the created file /usr/share/binfmts/qemu-arm and the commands update-binfmts
--import qemu-arm and update-binfmts --enable qemu-arm).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 23911c9e-9472-4915-8474-2947dc33f7b5
📒 Files selected for processing (1)
lib/functions/rootfs/qemu-static.sh
…n arch-test gate `arch-test arm` probes ARMv5 EABI, divergent from kernel COMPAT (armhf EABI v5+), and returns failure on modern aarch64 hosts (Ubuntu Noble 6.8 / Ampere Altra) with a fully working CONFIG_COMPAT path — routing mmdebstrap through qemu-user emulation at ~10× slowdown. Replace with a 4-tier detection: prefer the packaged `/usr/share/binfmts/qemu-arm` descriptor if present, trust an already-enabled kernel registration, otherwise probe COMPAT directly by exec'ing `ld-linux-armhf.so.3`, falling back to the hand-rolled descriptor only when none applies. Assisted-by: Claude:claude-opus-4.7
iav
force-pushed
the
fix/native-armhf-detection
branch
from
1aed440 to
1d6d472
Compare
2 participants
Problem
prepare_host_binfmt_qemu_cross_arm64_host_armhf_target(added in6755e9190, 2024-12-28, for Apple Silicon) gates onarch-test arm. On success → skip qemu-arm import; on failure → write a hand-rolled/usr/share/binfmts/qemu-armmagic with interpreter/usr/bin/qemu-arm-staticand enable it.This is broken on most modern aarch64 hosts:
arch-test armprobes ARMv5 EABI. Current toolchains and the kernel's COMPAT layer target armhf (EABI v5+); the two surfaces diverge.arch-test armreturns failure even withCONFIG_COMPAT=yand a fully-working COMPAT path. The gate then unconditionally routes every aarch64 host through the Apple-Silicon import branch — qemu-arm gets registered, mmdebstrap and chroot apt-get / dpkg --configure / customize_image run through qemu-user-static emulation, ~10× slower than native.update-binfmts --enable qemu-armat function entry could re-enable a pre-existing registration even on hosts the operator deliberately wanted to keep without qemu./usr/share/binfmts/qemu-arm(resolute'sqemu-user-binfmtships/usr/bin/qemu-armwithout-static) → enable fails → armhf builds break on resolute Apple-Silicon-like hosts.Empirically: helios4
BUILD_MINIMAL=yes RELEASE=trixie PREFER_DOCKER=yesfinishes in 1:07 min on Hetzner CAX21 when qemu-arm stays unregistered (mmdebstrap native via binfmt_elf), vs ~10× longer when emulation routes mmdebstrap.Fix
Replace the single
arch-test armgate with a 4-tier detection that picks the safest path on each host:/usr/share/binfmts/qemu-armexists (packaged byqemu-user-binfmton resolute, or by qemu-user-static install):update-binfmts --enable qemu-arm— loads the packaged descriptor into the kernel. Preserves the packaged interpreter path. Hard-error if enable fails (descriptor present but broken — host needs operator attention)./proc/sys/fs/binfmt_misc/qemu-armexists without descriptor: trust ifenabled, hard-error if disabled (no descriptor to re-enable from).exec /usr/arm-linux-gnueabihf/lib/ld-linux-armhf.so.3 --help(ld-linux-armhf is shipped bygcc-arm-linux-gnueabihf, an armbian aarch64 host build dep). Authoritative test ofCONFIG_COMPATfunctionality. If succeeds → kernel runs armhf natively → no qemu setup needed./usr/share/binfmts/qemu-armmagic with/usr/bin/qemu-arm-static, import, enable — the original behaviour, now only reached when actually needed.Also removed the unconditional
update-binfmts --enable qemu-arm "|| true"at function entry — its job is subsumed by step (1).Empirical validation
opts_n+=("COMPAT" "COMPAT_VDSO" "ARM64_32BIT_EL0")):ld-linux-armhf.so.3execs withExec format error; probe correctly fails → step (4) fires./usr/share/binfmts/qemu-armwith/usr/bin/qemu-arm-static): step (1) early-returns, descriptor md5 unchanged before/after, kernel entry left enabled.Test plan
qemu-user-binfmtpackage — covered by inspection (step 1 enables packaged descriptor). No resolute Apple Silicon hardware available; happy to defer to community verification.Summary by CodeRabbit
Bug Fixes
Refactor